feat: NSMul/NPow type class#38036
feat: NSMul/NPow type class#38036JovanGerb wants to merge 13 commits intoleanprover-community:masterfrom
NSMul/NPow type class#38036Conversation
PR summary 044631201fImport changes for modified filesNo significant changes to the import graph Import changes for all files
|
| Current number | Change | Type |
|---|---|---|
| 6395 | 1 | backward.isDefEq.respectTransparency |
| 803 | -2 | backward.privateInPublic |
| 414 | -1 | backward.privateInPublic.warn |
| 46 | -1 | dsimp +instances |
Current commit 12eaeeacf4
Reference commit 044631201f
You can run this locally as
./scripts/reporting/technical-debt-metrics.sh pr_summary
- The
relativevalue is the weighted sum of the differences with weight given by the inverse of the current value of the statistic. - The
absolutevalue is therelativevalue divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).
|
Could you elaborate on the benefit here? |
This can also be achieved with |
|
Sure, but then you lose the ability to write |
| ⟨fun x n ↦ NPow.npow n x⟩ | ||
|
|
||
| @[to_additive ofSMul] | ||
| instance NPow.ofPow {M : Type*} [Pow M ℕ] : NPow M := ⟨fun n x ↦ x ^ n⟩ |
There was a problem hiding this comment.
Is this desirable? Shouldn't we just assume that any Pow M ℕ instance already comes from NPow M?
| @[to_additive] instance addCommMonoid : AddCommMonoid R[M] := | ||
| fast_instance% { (inferInstance : AddCommMonoid <| M →₀ R) with | ||
| nsmul n x := x.mapRange (n • ·) (smul_zero _) } | ||
| inferInstanceAs <| AddCommMonoid <| M →₀ R |
There was a problem hiding this comment.
What happened to fast_instance%?
There was a problem hiding this comment.
This is the great thing: inferInstanceAs will automatically fill in the nsmul field using the smul that was was declared above. (Recall that inferInstanceAs and fast_instance% do the same kind of instance normalization). Previously, we had to awkwardly declare the nsmul field in order to avoid diamond.
I think this should also answer your other question: yes we do need to infer NSMul instances from SMul instances, for example here.
|
!bench |
|
Benchmark results for 12eaeea against 0446312 are in. There are significant results. @JovanGerb
Large changes (1✅, 4🟥)
Medium changes (16✅, 10🟥) Too many entries to display here. View the full report on radar instead. Small changes (39✅, 69🟥) Too many entries to display here. View the full report on radar instead. |
This PR is adds
NSMul,NPow,ZSMulandZPowclasses for thensmul,npow,zsmul,zpowdata fields.This has a few advantages:
SMulinstance, then you don't need to manually writensmul := (· • ·)andzsmul := (· • ·). ForPow, the extra benefit is that inferring the instance is preferred over the default fieldnpowRecAuto. So this helps avoid accidental diamonds.SMulinstance on a type synonym, theninferInstanceAswill infer thensmulfield from theSMulinstance. This makes it easier to avoid diamonds on type synonyms likeMatrixandMonoidAlgebra.NSMulandSMulclasses do not agree.In the process of making this PR, I have identified two existing
NPowdiamonds:Mathlib.Algebra.Order.Positive.Field, there were two conflictingNPowinstances.Fin, there are two conflictingNPowinstances. I have overwritten the one in core lean with the one from mathlib that is more computationally efficient.TODO: the same for
QSMulandNNQSMul.